-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
In CI print logs only when any test in a class fails #10620
In CI print logs only when any test in a class fails #10620
Conversation
if (memoryHandler == null) { | ||
return; | ||
} | ||
if (context.getFailedTests().size() != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an equivalent to this work for JUnit 5? Otherwise, we’ll lose this functionality when we migrate these tests to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check. I was not aware if we prefer JUnit 5 or TestNG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a TestWatcher. Is there any existing JUnit 5 test that has a Query Runner or logs something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We’re trying to move to JUnit 5: #9378
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AbstractTestQueryFramework
fundamentally requires TestNG due to the annotations in the base class, so this would have to be updated when we migrate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, but I wouldn’t want to add dependencies that will make it harder or introduce a roadblock to migrating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, maybe I can start the migration now? Would this be a good enough reason?
oldLevels.put(handler, handler.getLevel()); | ||
handler.setLevel(Level.OFF); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging is global, and tests execute in parallel.
- Concurrently to a test extending from ATQF, there might be some other test that doesn't, and we switch its logs off
- when two instances of ATQF initialize concurrently, the second one may record
oldLevels
asOFF
s, and dutifully restore them. This would disable logging for ever, for all tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What controls this? I naively thought parallelism is limited to classes: https://github.com/trinodb/trino/blob/master/pom.xml#L79
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methods
probably means that any two methods can run in parallel, so including two methods for same class, for same test instance, and for different classes and instances
classes
probably means that methods from a single class are not supposed to run in parallel (not sure how this plays with class hierarchies)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this article that has some examples with multiple classes and looks like you're right. I'll close this PR then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i weren't right at least to some extent, i wouldn't spend time on apache/maven-surefire#407 :)
The goal of this change is to produce fewer logs and make it easier to find the root cause of a failure. The suggested solution is to skip logs of test classes where all tests succeeded.
Supersedes #10592. It's better, because:
It works on the class level to allow running tests in parallel on a method level. So huge test classes that produce lots of logs will still have the same issue, but it'll reduce the noise if a failure happens in some other smaller test class in the same module.